Skip to content

User Settings: Add Shell selector dropdown to the Preferences tab #1216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 16, 2025
Merged

User Settings: Add Shell selector dropdown to the Preferences tab #1216

merged 4 commits into from
Apr 16, 2025

Conversation

gavande1
Copy link
Contributor

@gavande1 gavande1 commented Apr 16, 2025 •
edited
Loading

Related issues

  • Fixes STU-360

Proposed Changes

  • Add terminal shell selector dropdown on preferences tab
  • This PR does not handle:
    • Saving preference is app data.
    • Looking for actually installed terminals
    • Disable dropdown when none of the supported terminal is found.

Testing Instructions

  • Check out this branch
  • Run STUDIO_PREFERRED_EDITOR=true npm start
  • Open settings
  • Goto Preference Tab
  • Make sure the terminal dropdown is visible
  • Change the value and notice that the Save button is enabled.
  • You will be alerted with saved value when clicked on Save.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@gavande1 gavande1 changed the base branch from trunk to update/refactor-user-settings-module April 16, 2025 09:51
@gavande1 gavande1 marked this pull request as draft April 16, 2025 09:52
@gavande1 gavande1 requested review from gcsecsey and removed request for gcsecsey April 16, 2025 09:52
@gavande1 gavande1 marked this pull request as ready for review April 16, 2025 09:59
@gavande1 gavande1 requested a review from a team April 16, 2025 09:59
katinthehatsite
katinthehatsite approved these changes Apr 16, 2025
View reviewed changes
Copy link
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good and works as expected. Some minor comments but we can handle the followup with modal cleanup in a separate issue

@gavande1 gavande1 changed the base branch from update/refactor-user-settings-module to trunk April 16, 2025 13:37
@gavande1 gavande1 changed the base branch from trunk to update/refactor-user-settings-module April 16, 2025 13:38
gcsecsey
gcsecsey approved these changes Apr 16, 2025
View reviewed changes
Copy link
Contributor

@gcsecsey gcsecsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM and work great

I have a minor comment: when using the default window size the app starts with, the preferences tab now overflows its original size, so the buttons aren't fully visible:
CleanShot 2025-04-16 at 15 11 55@2x

I think a good solution to this would be to reduce the margin between the sections, but we can do this in a follow-up.

@gavande1 gavande1 force-pushed the add/preferred-terminal-dropdown branch from 43b4a2b to e490afc Compare April 16, 2025 14:33
@gavande1 gavande1 changed the base branch from update/refactor-user-settings-module to trunk April 16, 2025 14:33
@gavande1
Copy link
Contributor Author

gavande1 commented Apr 16, 2025 •
edited
Loading

Thanks @gcsecsey for pointing that out. We discussed it with @katinthehatsite and will handle it as part of STU-374.

@gavande1 gavande1 merged commit 963b833 into trunk Apr 16, 2025
8 checks passed
@gavande1 gavande1 deleted the add/preferred-terminal-dropdown branch April 16, 2025 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katinthehatsite katinthehatsite katinthehatsite approved these changes

@gcsecsey gcsecsey gcsecsey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gavande1 @gcsecsey @katinthehatsite